-
Notifications
You must be signed in to change notification settings - Fork 4.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve Filebeat organisiation and Cleanup #1894
Conversation
Yes. Potential side effect is, publisher still returning ACKed lines, the registrar can not handle anymore. Effect is lines need to be send again one restart. In general I like to treat pipelines being setup, configured + stopped in a stack like manner (makes it easier to reason about potential resource usage between stages). Didn't check yet, but when stopping the registrar, is some channel that has been used by publisher to forward results to registrar being closed? In this case we're asking for a panic on shutdown. |
done chan struct{} | ||
crawler *crawler.Crawler | ||
pub publish.LogPublisher | ||
done chan struct{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
uh, I like how this struct has become smaller. Can we rename FbConfig to Config at some point?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it also hurts me every time I see the variables ;-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed to config
. We have beatConfig
in the generator. But I now even prefer config.
In general I really like the changes. But start/stop handling in |
c9c1a52
to
1ecd4a4
Compare
// Start publisher | ||
publisher.Start() | ||
// Stopping publisher (might potentially drop items) | ||
defer publisher.Stop() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
with spooler being stopped before publisher, we really might drop items? Hmm... maybe if publish_async: true
in config file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We now have the flexibility to make optimizations here if needed. Currently the logic should be identical to what we had before.
The overall goal is to decouple all the modules to make them better reusable and extendable. In addition the current organisation was not very intuitive. * Move registrar to its own package * Improve startup handling of filebeat * Rename state to states in registrar as this is more accurate * Add warn message for truncated files. See https://github.com/elastic/beats/pull/1882/files#r67523587 * Move ignore older to ProspectorLog as only used there * Remove Prospector reference from ProspectorStdin * Remove empty filebeat test file * Cleanup New function naming * Implement defer statements to stop running services * Clean up crawler / prospector stopping
1ecd4a4
to
6079993
Compare
LGTM. Let's wait for travis/appveyor |
The overall goal is to decouple all the modules to make them better reusable and extendable.
In addition the current organisation was not very intuitive.